Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround for type inference problem in index_sizes #139

Merged
merged 3 commits into from
Apr 15, 2017

Conversation

martinholters
Copy link
Collaborator

Wokraround for JuliaLang/julia#21244.

I briefly tried adding a test case similar to the one from JuliaLang/julia#21244, but it seems to need the struct Foo which cannot be defined inside a @testset. Is it worth to define such a type globally in runtests.jl?

@martinholters
Copy link
Collaborator Author

Ah, just after (of course!) submitting the PR, I realized two things:

  1. The new index_sizes(s::Size, inds...) will error if s contains fewer elements than inds. Is that a problem? The fix should be trivial.
  2. code_warntype(DevNull, StaticArrays.index_sizes, Tuple{Vararg{Any}}) is able to trigger the error so would make a good test. Update incoming...

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 69.605% when pulling 81c0a22 on martinholters:mh/index_sizes into 8d3ec32 on JuliaArrays:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 69.652% when pulling bfe0273 on martinholters:mh/index_sizes into 8d3ec32 on JuliaArrays:master.

@martinholters
Copy link
Collaborator Author

Fixed the the case for s containing fewer elements than inds (in case that matters).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 69.714% when pulling 09a86d0 on martinholters:mh/index_sizes into 8d3ec32 on JuliaArrays:master.

@andyferris
Copy link
Member

Thanks for working on this!

I'm on vacation for another week, so I can't have an in depth look at the moment. I can say I'm very happy this is being looked at.

I assume someone has (or will) check the generated code? Other than that, I think I'm ok with this PR.

@martinholters
Copy link
Collaborator Author

I don't think I'm sufficiently familiar with this package to check the generated code. I've done some very basic benchmarking of indexing:

sm = @SMatrix [1 2 3; 4 5 6]
sv = @SVector [1, 2]
@benchmark $sm[$sv, 1]
@benchmark $sm[1, $sv]
@benchmark $sm[:, $sv]
@benchmark $sm[$sv, :]
@benchmark $sm[1, :]
@benchmark $sm[1, 1]
@benchmark $sm[:, 1]
@benchmark $sm[:, :]

They are all within measurement between master and this PR. I've also checked @code_warntype sm[:, 1] as one example and it is identical between master and this PR. But these probably aren't the critical cases?

@c42f
Copy link
Member

c42f commented Apr 13, 2017

Thanks for tackling this one. It's very pleasing that your new implementation avoids any mention of Val :-)

Regarding your struct Foo problem, the way I've been dealing with this is as follows:

@testset "blah" begin

# ... lots of other tests ...

# Now the test which needs a custom type
@eval struct Foo
    i::Int
end

# Some test using Foo
@test Foo(1).i == 1

end

Of course the type Foo is still defined globally within the current module, but at least the code for it is next to the test which needs it.

Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent a while looking at this (some benchmarking and reading various pieces of assembly). It seems to function perfectly as far as I can tell.

So I'll probably merge this tomorrow. @martinholters - did you want to add that other test case?

@martinholters
Copy link
Collaborator Author

I don't think it would be worth the effort, so: no. If anyone disagrees, I'd be happy to add it, but not in the next couple of days.

@c42f c42f merged commit b643ff5 into JuliaArrays:master Apr 15, 2017
@c42f
Copy link
Member

c42f commented Apr 15, 2017

Sure no problems. I had a quick look but it seems to be obscure behavior which would be better as a compiler test.

@martinholters martinholters deleted the mh/index_sizes branch April 15, 2017 07:47
@c42f c42f mentioned this pull request Apr 18, 2017
oschulz pushed a commit to oschulz/StaticArrays.jl that referenced this pull request Apr 4, 2023
* allow failures on nightly

* also test julia 1.5

* drop unsupported releases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants